Skip to content

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 18, 2025

This is the second part of tests/integration cleanup I wanted to do for quite some time.
For the first part, see #4945.

  1. Improve runc wrapper

    1. Add status check support (same as in bats' run helper).

    2. Add PRE_CMD support (so we can use commands like taskset or timeout).

    3. Drop sane_helper since the output of the command is shown in case of
      an error, and we show the command itself in runc wrapper (unless -N
      or ! is provided -- in this case the command is shown by bats,
      together with the error). This does not show the output of successful
      commands which IMO is a net positive since we are almost always
      interested in failed command output only.

    4. Use the new functionality in cpu_affinity.bats and start.bats as a
      showcase (the test of refactoring is in a separate commit).

  2. refactor to use runc status checks

  3. add missing runc status checks

@kolyshkin
Copy link
Contributor Author

CI failure in almalinux9 is a known flake #4437. Restarted.

@kolyshkin
Copy link
Contributor Author

Now we do:

runc start foo
[ $status -eq 0 ]

and runc hides a call to run.

With this PR, we now do

runc -0 start foo

I.e. we still hide run inside but allow runc to accept some of run options (-N and ! for now).

The alternative to that is use run explicitly:

run -0 runc start foo

I am open to any suggestions.

1. Add status check support (same as in bats' run helper).

2. Add RUNC_PRE_CMD support (so we can use commands like taskset or
   timeout).

3. Drop sane_helper since the output of the command is shown in case of
   an error, and we show the command itself in runc wrapper (unless -N
   or ! is provided -- in this case the command is shown by bats,
   together with the error). This does not show the output of successful
   commands which IMO is a net positive since we are almost always
   interested in failed command output only.

4. Use the new functionality in cpu_affinity.bats and start.bats as a
   showcase (the test of refactoring is in a separate commit).

Signed-off-by: Kir Kolyshkin <[email protected]>
These places should check runc exit code but they don't.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin marked this pull request as ready for review October 26, 2025 22:21
@kolyshkin kolyshkin requested review from cyphar, lifubang and rata and removed request for rata October 26, 2025 22:21
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@rata
Copy link
Member

rata commented Oct 30, 2025

The alternative to that is use run explicitly:

run -0 runc start foo

I like this better, it's also trivial to add taskset, etc. I think having a bash function named runc (instead of runc being the binary) is just confusing and it doesn't really add much. IMHO if we want such function, we should call it something like rrun.

Can we use run instead of the env var for the taskset commands?

But if you or others prefers as it is in this PR now, I'm fine too :)

@kolyshkin
Copy link
Contributor Author

The alternative to that is use run explicitly:

run -0 runc start foo

I like this better, it's also trivial to add taskset, etc. I think having a bash function named runc (instead of runc being the binary) is just confusing and it doesn't really add much. IMHO if we want such function, we should call it something like rrun.

Can we use run instead of the env var for the taskset commands?

But if you or others prefers as it is in this PR now, I'm fine too :)

I'd prefer run -0 runc start foo over whatever we have now or this PR, even if it's more verbose (mostly because no one understands that runc in bats means run runc ....

One issue is, we can't mix bash functions and binaries together, IOW something like run -0 taskset xxx runc start foo won't work.

I'll try to come up with something better.

@cyphar
Copy link
Member

cyphar commented Oct 31, 2025

Drop sane_helper since the output of the command is shown in case of an error, and we show the command itself in runc wrapper (unless -N or ! is provided -- in this case the command is shown by bats, together with the error). This does not show the output of successful commands which IMO is a net positive since we are almost always interested in failed command output only.

I have often found when debugging test failures that successful output is quite useful -- not least of which when the failing line is [[ "$output" == "foo" ]]. Even if you migrated all of the tests to \| grep -Fx foo, you still wouldn't be able to capture that information in the failing case. Maybe if there was a helper for this that only output on failure? (But then we're sacrificing readability again.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants